[19.0][MIG] sale_pricelist_triple_discount: Migration to 19.0 #4088
[19.0][MIG] sale_pricelist_triple_discount: Migration to 19.0 #4088
Conversation
|
/ocabot migration sale_pricelist_triple_discount |
4587703 to
aeddb1b
Compare
Currently translated at 100.0% (6 of 6 strings) Translation: sale-workflow-8.0/sale-workflow-8.0-sale_pricelist_triple_discount Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-8-0/sale-workflow-8-0-sale_pricelist_triple_discount/it/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: sale-workflow-16.0/sale-workflow-16.0-sale_pricelist_triple_discount Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-16-0/sale-workflow-16-0-sale_pricelist_triple_discount/
Currently translated at 100.0% (11 of 11 strings) Translation: sale-workflow-16.0/sale-workflow-16.0-sale_pricelist_triple_discount Translate-URL: https://translation.odoo-community.org/projects/sale-workflow-16-0/sale-workflow-16-0-sale_pricelist_triple_discount/it/
aeddb1b to
56a5e3d
Compare
alexey-pelykh
left a comment
There was a problem hiding this comment.
Heads up: CI is failing on both Odoo and OCB test runs, and runboat is also down. Might want to check that before asking for reviews.
From a code perspective, the _patch_triple_discount() context manager pattern is interesting — temporarily mutating the discount field via sudo() for the duration of _compute_price and then restoring it. It works, but it's fragile if an exception occurs between the yield and the restore (the finally block is missing). Consider wrapping the restore in a try/finally to be safe.
| for item, original_discount in item_to_original_discount.items(): | ||
| item_discount_field = COMPUTE_PRICE_TO_DISCOUNT_FIELD.get( | ||
| item.compute_price | ||
| ) |
There was a problem hiding this comment.
The restore logic after yield should be inside a try/finally block. If _compute_price (or any caller) raises, the original discounts won't be restored, leaving the records in a modified state.
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for working on this migration.
I found several issues that need to be addressed before this can move forward:
_compute_name_and_price no longer exists in Odoo 19.0. This method was split into _compute_name() and _compute_price_label(). The override in pricelist.py is dead code and will never be called, which means the price field on pricelist items won't recompute when discount2 or discount3 change. You should override _compute_price_label instead of _compute_name_and_price.
_compute_discounts hardcodes price_rule.percent_price for all rule types. When compute_price == "formula", the relevant field is price_discount, not percent_price. Currently for formula rules, discount1 would be set to 0 instead of the actual formula discount. The fix is straightforward -- use price_rule[item_discount_field] instead of price_rule.percent_price.
Missing discount_policy guard in _compute_discounts. The 16.0 version only propagated discounts to the order line when pricelist.discount_policy == "without_discount". That check was dropped in the migration, so discounts would be incorrectly propagated even for "with_discount" pricelists (where they should remain zero since the discount is already baked into the unit price).
_patch_triple_discount still lacks a try/finally around the restore block (same point from my earlier review). If anything between the yield and the restore raises, the original discount values are never written back. Wrapping the yield+restore in try/finally would make this safe.
Minor: the discount_fields += ["discount"] line in _get_triple_discounts_perc adds a field that doesn't exist on product.pricelist.item -- it gets silently ignored by fields_get. Looks like a leftover from adapting the _discount_fields() return value change ("discount" -> "discount1"). Harmless but confusing, worth cleaning up.
Also, CI is still failing so none of the tests have been verified. The test expectations look correct for the intended behavior, but the implementation bugs above would cause test_formula_without_discount and test_percentage_with_discount to fail once CI is green.
Based on #3401
Depends on: